Skip to content

[Instrumentation.AspNet] Improved route template normalization performance #3241

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Oct 17, 2025
Merged

[Instrumentation.AspNet] Improved route template normalization performance #3241

merged 4 commits into from
Oct 17, 2025

Conversation

@matt-hensley
Copy link
Contributor

@matt-hensley matt-hensley commented Oct 16, 2025 •
edited
Loading

Changes

Reworked route template normalization from #3160 to improve CPU and memory usage.

Thanks for sharing the benchmark code @RassK!

BenchmarkDotNet v0.15.4, Windows 11 (10.0.26100.6584/24H2/2024Update/HudsonValley)
AMD Ryzen 5 PRO 6650U with Radeon Graphics 2.90GHz, 1 CPU, 12 logical and 6 physical cores
  [Host]     : .NET Framework 4.8.1 (4.8.9310.0), X86 LegacyJIT
  DefaultJob : .NET Framework 4.8.1 (4.8.9310.0), X86 LegacyJIT


| Method                      | Mean     | Error     | StdDev    | Ratio | RatioSD | Gen0       | Allocated | Alloc Ratio |
|---------------------------- |---------:|----------:|----------:|------:|--------:|-----------:|----------:|------------:|
| PrepareRouteTemplate_String | 3.077 ms | 0.0166 ms | 0.0147 ms |  1.00 |    0.01 |  2539.0625 |   2.33 MB |        1.00 |
| PrepareRouteTemplate_Alt    | 1.413 ms | 0.0054 ms | 0.0050 ms |  0.46 |    0.00 |  1541.0156 |   1.41 MB |        0.61 |

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@github-actions github-actions bot added the comp:instrumentation.aspnet Things related to OpenTelemetry.Instrumentation.AspNet label Oct 16, 2025
@codecov
Copy link

codecov bot commented Oct 16, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.15%. Comparing base (3a5b8ee) to head (1d083c7).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3241      +/-   ##
==========================================
+ Coverage   69.92%   70.15%   +0.22%     
==========================================
  Files         440      430      -10     
  Lines       16974    16935      -39     
==========================================
+ Hits        11869    11880      +11     
+ Misses       5105     5055      -50
Flag Coverage Δ
unittests-Instrumentation.AspNet 77.92% <100.00%> (+0.68%) ⬆️
unittests-Instrumentation.Cassandra ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...on.AspNet/Implementation/HttpRequestRouteHelper.cs 100.00% <100.00%> (ø)

... and 15 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@matt-hensley matt-hensley marked this pull request as ready for review October 16, 2025 14:35
@matt-hensley matt-hensley requested a review from a team as a code owner October 16, 2025 14:35
martincostello
martincostello approved these changes Oct 16, 2025
View reviewed changes
@martincostello martincostello requested a review from RassK October 17, 2025 08:15
Kielek
Kielek approved these changes Oct 17, 2025
View reviewed changes
Copy link
Member

@Kielek Kielek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Potential follow up: Do you see any place when we can even more drop memory allocation by some Spans?

@Kielek Kielek merged commit 8b5340d into open-telemetry:main Oct 17, 2025
61 checks passed
@matt-hensley
Copy link
Contributor Author

Potential follow up: Do you see any place when we can even more drop memory allocation by some Spans?

Yes, I created a prototype using Span that showed significantly lower allocations (~5% of baseline) while the CPU usage was only slightly lower. With additional work, it would be worth taking the dependency on the System.Memory package for netfx.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@martincostello martincostello martincostello approved these changes

@Kielek Kielek Kielek approved these changes

+1 more reviewer

@RassK RassK RassK approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

comp:instrumentation.aspnet Things related to OpenTelemetry.Instrumentation.AspNet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@matt-hensley @martincostello @RassK @Kielek